refactor: consolidate reference tracking with Tracker in deployer#3687
Conversation
…ructure - Replaced individual sets for referenced secrets, config maps, and PVCs with a Tracker that encapsulates these references. - Updated the generateDeployment and CheckResourcesArePresent functions to utilize the Tracker, ensuring consistent resource validation. - Modified related tests to verify that the Tracker correctly accumulates resource references during deployment generation. This change enhances the clarity and maintainability of the code by consolidating resource tracking logic.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Ankitsinghsisodya The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @Ankitsinghsisodya. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
There was a problem hiding this comment.
Pull request overview
Refactors reference tracking for Kubernetes resource validation by introducing a k8s.Tracker/k8s.References shape, and threads it through the k8s and knative deployers so referenced Secrets/ConfigMaps/PVCs are accumulated consistently during manifest generation and then validated.
Changes:
- Added
ReferencesandTracker(withNewTracker()) to consolidate the three reference sets into a single accumulator. - Converted env/volume processing helpers to
*Trackerreceiver methods and updated deploy paths to pass a tracker through generation and validation. - Updated knative and k8s unit tests to use
NewTracker()and to assert againsttracker.Secrets/tracker.ConfigMaps.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/k8s/deployer.go | Introduces References/Tracker, refactors env/volume processing and resource validation APIs to use the tracker. |
| pkg/knative/deployer.go | Threads *k8s.Tracker through service generation and resource validation on create/update paths; removes unused sets import. |
| pkg/k8s/deployer_test.go | Updates deployment-generation tests to pass a Tracker instead of three sets. |
| pkg/knative/deployer_test.go | Updates regression test to use NewTracker() and assert tracked references via tracker fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3687 +/- ##
==========================================
- Coverage 56.90% 56.11% -0.79%
==========================================
Files 181 181
Lines 20933 20974 +41
==========================================
- Hits 11912 11770 -142
- Misses 7811 8033 +222
+ Partials 1210 1171 -39
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ssVolumes functions - Introduced ensureInit method in Tracker to initialize nil sets for Secrets, ConfigMaps, and PVCs, preventing panics on nil-map Insert. - Updated ProcessEnvs and ProcessVolumes functions to call the new Tracker methods, ensuring backward compatibility while encouraging the use of the Tracker directly. - Improved error messages in knative deployer for resource validation failures, enhancing clarity in error reporting. This change improves the robustness of resource tracking and prepares for future refactoring by phasing out deprecated functions.
What
Fixes #3678
Fixes the three code smells called out in #3678 by introducing a
Trackerstruct (shape b from the issue) that owns the three reference sets and
accumulates them via receiver methods.
Changes
pkg/k8s/deployer.goReferencesstruct holdingSecrets,ConfigMaps,PVCsassets.Set[string]values (not pointers — maps are already reference types).Trackerstruct embeddingReferencesandNewTracker()constructorthat initialises all three sets.
ProcessEnvs,ProcessVolumes,createEnvFromSource,createEnvVarSourcefrom free functions with*sets.Set[string]out-parameters to
*Trackermethods that write directly into the receiver.generateDeploymentto accept*Trackerinstead of three pointerparameters.
CheckResourcesArePresentto acceptReferencesby value instead ofthree
*sets.Set[string]parameters.Deploy()(create and update paths).pkg/knative/deployer.gogenerateNewServicesignature to accept*k8s.Tracker.Deploy()(create and update paths).setsimport.pkg/k8s/deployer_test.go/pkg/knative/deployer_test.gosets.New[string]()+ pointer passing withNewTracker()in all affected tests.tracker.Secrets/tracker.ConfigMaps.setsimports.Why shape (b) — Tracker receiver
can silently create a disconnected set and pass it alongside the one that
gets validated; with a single Tracker receiver there is nothing to
disconnect.
sets.Set[string]ismap[string]Empty; passing it by value insideReferencesalready letscallees mutate the same backing store without the extra
*.Behavior
No behavior changes. This is a pure shape refactor.
Test plan
go test ./pkg/k8s/ ./pkg/knative/— both passmake check— 0 lint issuesmake test— all pre-existing unit tests pass